Upgrade analyzer version to 0.1.1-dev.20230613.1#37037
Upgrade analyzer version to 0.1.1-dev.20230613.1#37037pshao25 wants to merge 27 commits intoAzure:mainfrom
Conversation
pshao25
requested review from
benbp,
hallipr,
mengaims and
weshaggard
as code owners
pshao25
requested review from
ArcturusZhang,
ArthurMa1978,
JialinXin,
JoshLove-msft,
KrzysztofCwalina,
NiveditJain,
ShivangiReja,
amnguye,
annelo-msft,
archerzz,
chrissmiller,
christothes,
jaschrep-msft,
mharlan,
nisha-bhatia,
pallavit,
schaabs,
seanmcc-msft,
sebrenna,
tg-msft,
vicancy and
wonner
as code owners
|
Pull request contains merge conflicts. |
|
/azp run net - compute - mgmt |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| #pragma warning disable AZC0004 // DO provide both asynchronous and synchronous variants for all service methods. | ||
| public virtual AsyncPageable<AssetConversion> GetConversionsAync(CancellationToken cancellationToken = default) | ||
| #pragma warning restore AZC0004 // DO provide both asynchronous and synchronous variants for all service methods. | ||
| { |
There was a problem hiding this comment.
There is a typo here GetConversionsAync should be GetConversionsAsync. Therefore these two async/sync pair cannot find each other. I have to suppress here. This library is GAed.
There was a problem hiding this comment.
Thank you for finding this! It might be helpful to file an issue for this library to let them know - they could introduce a correctly-spelled method and mark the misspelled one with "EditorBrowsableState.Never"
| public virtual async Task<Response<BlobDownloadInfo>> DownloadAsync( | ||
| CancellationToken cancellationToken) => | ||
| CancellationToken cancellationToken = default) => | ||
| await DownloadAsync( |
There was a problem hiding this comment.
It's corresponding sync method Download has optional CancellationToken.
| /// </remarks> | ||
| public virtual async Task<Response<BlobDownloadResult>> DownloadContentAsync( | ||
| CancellationToken cancellationToken) => | ||
| CancellationToken cancellationToken = default) => |
There was a problem hiding this comment.
It's corresponding sync method DownloadContent has optional cancellationToken.
| #pragma warning disable AZC0018 // Protocol method should take a RequestContext parameter called `context` and not use a model as parameter or return types. | ||
| public virtual async Task<Response<bool>> GroupExistsAsync(string group, RequestContext context = default) | ||
| #pragma warning restore AZC0018 // Protocol method should take a RequestContext parameter called `context` and not use a model as parameter or return types. | ||
| { |
There was a problem hiding this comment.
I don't know why there is a RequestContext and a Response<bool>. But this one is GAed. I have to suppress.
There was a problem hiding this comment.
I think we allow this one for HEAD requests -- it would be worth checking the generator code to confirm my memory that this is what the generator will write in all cases for a HEAD request . It might make sense to update the PR for this so it doesn't confuse client authors in the future, but I'm happy for us to track this as future work in a separate issue.
There was a problem hiding this comment.
Yes, you are correct. I fixed in this PR.
| /// A <see cref="RequestFailedException"/> will be thrown if | ||
| /// a failure occurs. | ||
| /// </remarks> | ||
| #pragma warning disable AZC0002 // Client method should have an optional CancellationToken (both name and it being optional matters) or a RequestContext as the last parameter. |
There was a problem hiding this comment.
I am happy to see these being added in line, rather than in a GlobalSuppressions file, because it means the analyzer will still catch issues in the future. Out of curiosity, what has changed in the analyzer for AZC0002 that made it start catching these where it didn't before? For these overloads that call through to a method that takes a cancellation token, could we identify those in the analyzer? I think the goal of the analyzer is to ensure that there is an overload that takes a cancellation token in the public API.
There was a problem hiding this comment.
Yeah, I changed the logic to look for overloads. Then the suppression could be removed.
| /// A <see cref="RequestFailedException"/> will be thrown if | ||
| /// a failure occurs. | ||
| /// </remarks> | ||
| #pragma warning disable AZC0002 // Client method should have an optional CancellationToken (both name and it being optional matters) or a RequestContext as the last parameter. |
There was a problem hiding this comment.
Would it be possible to make this cancellationToken optional, or would that be a breaking change?
Why must the cancellationToken be optional if there is an overload that doesn't take one?
| /// A <see cref="RequestFailedException"/> will be thrown if | ||
| /// a failure occurs. | ||
| /// </remarks> | ||
| #pragma warning disable AZC0002 // Client method should have an optional CancellationToken (both name and it being optional matters) or a RequestContext as the last parameter. |
There was a problem hiding this comment.
These look like valid exemptions - do we know why the analyzer didn't catch this one before?
| public ChatClient(System.Uri endpoint, Azure.Communication.CommunicationTokenCredential communicationTokenCredential, Azure.Communication.Chat.ChatClientOptions options = null) { } | ||
| public virtual Azure.Response<Azure.Communication.Chat.CreateChatThreadResult> CreateChatThread(string topic, System.Collections.Generic.IEnumerable<Azure.Communication.Chat.ChatParticipant> participants, string idempotencyToken = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | ||
| public virtual System.Threading.Tasks.Task<Azure.Response<Azure.Communication.Chat.CreateChatThreadResult>> CreateChatThreadAsync(string topic, System.Collections.Generic.IEnumerable<Azure.Communication.Chat.ChatParticipant> participants = null, string idempotencyToken = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | ||
| public virtual System.Threading.Tasks.Task<Azure.Response<Azure.Communication.Chat.CreateChatThreadResult>> CreateChatThreadAsync(string topic, System.Collections.Generic.IEnumerable<Azure.Communication.Chat.ChatParticipant> participants, string idempotencyToken = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } |
There was a problem hiding this comment.
Is this a breaking change to a GA library?
There was a problem hiding this comment.
@tg-msft, can you confirm this is an acceptable change?
There was a problem hiding this comment.
Given that we're effectively replacing a runtime NullRefEx with a compile time break per https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/communication/Azure.Communication.Chat/src/ChatClient.cs#L59-L95, I think we should take this.
+@glorialimicrosoft as an FYI of the change and that we might also want an Argument.AssertNotNull(participants, nameof(participants)) here (which isn't considered breaking to add either).
|
Hi @pshao25. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
github-actions
Bot
added
the
no-recent-activity
6 participants
See changes at Azure/azure-sdk-tools#6496 (comment)